-
Notifications
You must be signed in to change notification settings - Fork 11k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mlir][OpenMP] - Honor dependencies in code-generation of the if clause in omp.task
correctly
#90891
Conversation
…se in omp.task correctly This patch fixes code generation of the if clause specifically when the condition evaluates to false and when the task directive has the depend clause on it. When the if clause of a task construct evaluates to false, then the task is an undeferred task. This undeferred task still has to honor dependencies. Previously, the OpenMPIRbuilder didn't honor dependencies. This patch fixes that. Fixes llvm#90869
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-mlir Author: Pranav Bhandarkar (bhandarkar-pranav) ChangesThis patch fixes code generation of the if clause specifically when the condition evaluates to false and when the task directive has the depend clause on it. When the if clause of a task construct evaluates to false, then the task is an undeferred task. This undeferred task still has to honor dependencies. Previously, the OpenMPIRbuilder didn't honor dependencies. This patch fixes that. Fixes #90869 Full diff: https://github.com/llvm/llvm-project/pull/90891.diff 2 Files Affected:
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 4d2d352f7520b2..695308419e0d20 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -1887,6 +1887,15 @@ OpenMPIRBuilder::createTask(const LocationDescription &Loc,
SplitBlockAndInsertIfThenElse(IfCondition, IfTerminator, &ThenTI,
&ElseTI);
Builder.SetInsertPoint(ElseTI);
+
+ if (Dependencies.size()) {
+ Function *TaskWaitFn =
+ getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_taskwait_deps_51);
+ Builder.CreateCall(TaskWaitFn, {Ident, ThreadID, Builder.getInt32(Dependencies.size()),
+ DepArray, ConstantInt::get(Builder.getInt32Ty(), 0),
+ ConstantPointerNull::get(PointerType::getUnqual(M.getContext())),
+ ConstantInt::get(Builder.getInt32Ty(), false)});
+ }
Function *TaskBeginFn =
getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_task_begin_if0);
Function *TaskCompleteFn =
diff --git a/mlir/test/Target/LLVMIR/omptask_if_false.mlir b/mlir/test/Target/LLVMIR/omptask_if_false.mlir
new file mode 100644
index 00000000000000..c7fd5eb62a9772
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/omptask_if_false.mlir
@@ -0,0 +1,23 @@
+// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<!llvm.ptr<270>, dense<32> : vector<4xi64>>, #dlti.dl_entry<i64, dense<64> : vector<2xi64>>, #dlti.dl_entry<i32, dense<32> : vector<2xi64>>, #dlti.dl_entry<!llvm.ptr<272>, dense<64> : vector<4xi64>>, #dlti.dl_entry<!llvm.ptr<271>, dense<32> : vector<4xi64>>, #dlti.dl_entry<f16, dense<16> : vector<2xi64>>, #dlti.dl_entry<f64, dense<64> : vector<2xi64>>, #dlti.dl_entry<f128, dense<128> : vector<2xi64>>, #dlti.dl_entry<f80, dense<128> : vector<2xi64>>, #dlti.dl_entry<i128, dense<128> : vector<2xi64>>, #dlti.dl_entry<i16, dense<16> : vector<2xi64>>, #dlti.dl_entry<i8, dense<8> : vector<2xi64>>, #dlti.dl_entry<i1, dense<8> : vector<2xi64>>, #dlti.dl_entry<!llvm.ptr, dense<64> : vector<4xi64>>, #dlti.dl_entry<"dlti.endianness", "little">, #dlti.dl_entry<"dlti.stack_alignment", 128 : i64>>, fir.defaultkind = "a1c4d8i4l4r4", fir.kindmap = "", llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128", llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_gpu = false, omp.is_target_device = false, omp.version = #omp.version<version = 11>}
+{
+ llvm.func @foo_(%arg0: !llvm.ptr {fir.bindc_name = "n"}, %arg1: !llvm.ptr {fir.bindc_name = "r"}) attributes {fir.internal_name = "_QPfoo"} {
+ %0 = llvm.mlir.constant(false) : i1
+ omp.task if(%0) depend(taskdependin -> %arg0 : !llvm.ptr) {
+ %1 = llvm.load %arg0 : !llvm.ptr -> i32
+ llvm.store %1, %arg1 : i32, !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+ }
+ llvm.func @llvm.stacksave.p0() -> !llvm.ptr attributes {sym_visibility = "private"}
+ llvm.func @llvm.stackrestore.p0(!llvm.ptr) attributes {sym_visibility = "private"}
+}
+
+
+// CHECK: call void @__kmpc_omp_taskwait_deps_51
+// CHECK-NEXT: call void @__kmpc_omp_task_begin_if0
+// CHECK-NEXT: call void @foo_..omp_par
+// CHECK-NEXT: call void @__kmpc_omp_task_complete_if0
+
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvm.func @llvm.stacksave.p0() -> !llvm.ptr attributes {sym_visibility = "private"} | ||
llvm.func @llvm.stackrestore.p0(!llvm.ptr) attributes {sym_visibility = "private"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: these are probably not necessary.
@@ -0,0 +1,23 @@ | |||
// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s | |||
|
|||
module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<!llvm.ptr<270>, dense<32> : vector<4xi64>>, #dlti.dl_entry<i64, dense<64> : vector<2xi64>>, #dlti.dl_entry<i32, dense<32> : vector<2xi64>>, #dlti.dl_entry<!llvm.ptr<272>, dense<64> : vector<4xi64>>, #dlti.dl_entry<!llvm.ptr<271>, dense<32> : vector<4xi64>>, #dlti.dl_entry<f16, dense<16> : vector<2xi64>>, #dlti.dl_entry<f64, dense<64> : vector<2xi64>>, #dlti.dl_entry<f128, dense<128> : vector<2xi64>>, #dlti.dl_entry<f80, dense<128> : vector<2xi64>>, #dlti.dl_entry<i128, dense<128> : vector<2xi64>>, #dlti.dl_entry<i16, dense<16> : vector<2xi64>>, #dlti.dl_entry<i8, dense<8> : vector<2xi64>>, #dlti.dl_entry<i1, dense<8> : vector<2xi64>>, #dlti.dl_entry<!llvm.ptr, dense<64> : vector<4xi64>>, #dlti.dl_entry<"dlti.endianness", "little">, #dlti.dl_entry<"dlti.stack_alignment", 128 : i64>>, fir.defaultkind = "a1c4d8i4l4r4", fir.kindmap = "", llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128", llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_gpu = false, omp.is_target_device = false, omp.version = #omp.version<version = 11>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can you remove the attributes that are not needed for the test to work?
if (Dependencies.size()) { | ||
Function *TaskWaitFn = | ||
getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_taskwait_deps_51); | ||
Builder.CreateCall( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider updating the comment starting with
In the presence of the `if` clause, the following IR is generated:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
if (Dependencies.size()) { | ||
Function *TaskWaitFn = | ||
getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_omp_taskwait_deps_51); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the nowait
argument passed is false, wouldn't __kmpc_omp_wait_deps
be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
@Meinersbur and @kiranchandramohan - Thank you for your review. I have addressed your comments. Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please wait for @Meinersbur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This patch fixes code generation of the if clause specifically when the condition evaluates to false and when the task directive has the depend clause on it. When the if clause of a task construct evaluates to false, then the task is an undeferred task. This undeferred task still has to honor dependencies. Previously, the OpenMPIRbuilder didn't honor dependencies. This patch fixes that.
Fixes #90869